Skip to content

feat(validation): lifecycle placement of dev warnings - #6561

Open
miwha-adobe wants to merge 9 commits into
ruben/feat-dev-warning-validation-componentsfrom
miwha/feat-dev-warning-refactors
Open

feat(validation): lifecycle placement of dev warnings#6561
miwha-adobe wants to merge 9 commits into
ruben/feat-dev-warning-validation-componentsfrom
miwha/feat-dev-warning-refactors

Conversation

@miwha-adobe

@miwha-adobe miwha-adobe commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Description

Moves pure property-value dev-warnings (validateEnum, plus StatusLight's deprecation warnings) from updated() (post-render) to the pre-render phase, so a warning about an invalid value fires before first paint instead of after. For seven components: Asset, Card, IllustratedMessage, ProgressCircle, StatusLight, Tooltip, ActionButton.

No behavior change. Only the timing of dev-mode warnings changes; rendered output, ARIA, and focus are untouched, and production is unaffected (dev warnings are stripped there). DOM-dependent checks stay in updated() by design (Card title-as-link / checkActionsSupport, ProgressCircle aria-label default).

The separate, contested fallback/coercion change is intentionally excluded and lives as a do-not-merge POC in #6564.

Dev-warning revalidation

Added an AttributeObserverController so accessibility warnings that read aria-label / aria-labelledby re-check when those change. Before, the check only ran once when the component connected, so removing a label afterward went unnoticed. It only runs in dev, so production is unaffected.

Also removed the _hasWarned flags from Dropzone and LinearProgressMixin. They looked like they re-showed a warning after fixing and re-breaking something, but the warning system already shows each message only once per session, so the flags weren't actually doing anything. Replaced them with a simple early return.

Motivation and context

A property-value check in updated() runs after the component has already rendered once with the invalid value. Running it pre-render surfaces the warning before paint and matches the style guide (validateEnum in update() before super.update()).

That pre-render approach works for reactive properties, but not for accessible names that live in aria-label / aria-labelledby: those aren't reactive, so changing them never triggers a render and the check goes stale after the first run. AttributeObserverController covers that case by observing those attributes and requesting a re-check when they change (dev-only, so production is unaffected).

While doing this, we also found the _hasWarned flags in Dropzone and LinearProgressMixin weren't doing anything: the warning system already dedups identical messages once per session, so the flags never re-surfaced a warning even though the code read as if they did. They've been removed in favor of a simple early return.

Related issue(s)

Screenshots (if appropriate)

N/A, no visual change.


Author's checklist

  • I have read the CONTRIBUTING and PULL_REQUESTS documents.
  • I have reviewed the Accessibility Practices for this feature, see: Aria Practices
  • I have added automated tests to cover my changes. (Existing suites cover it: 894 tests pass, typecheck clean.)
  • I have included a well-written changeset if my change needs to be published. (Not needed: dev-only, no published change.)
  • I have included updated documentation if my change required it. (Not required.)

Reviewer's checklist

  • Includes a Github Issue with appropriate flag or Jira ticket number without a link
  • Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
  • Automated tests cover all use cases and follow best practices for writing
  • Validated on all supported browsers
  • All VRTs are approved before the author can update Golden Hash

Manual review test cases

  • Invalid enum warns, with no visual change
    1. Run Storybook in dev mode.
    2. On any affected component (e.g. Card), set an invalid enum value such as variant="banana".
    3. Expect the console warning to fire and the rendering to match main.

Device review

  • Did it pass in Desktop?
  • Did it pass in (emulated) Mobile?
  • Did it pass in (emulated) iPad?

Accessibility testing checklist

Timing-only change; no roles, names, states, focus, or keyboard behavior are affected. Testing confirms no regression.

  • Keyboard: Tab to Action Button and Tooltip; confirm activation and tooltip show/hide behave as on main.
  • Screen reader: With VoiceOver, confirm Status Light and Progress Circle announce the same role and name as on main (including Progress Circle's default "Loading").

@changeset-bot

changeset-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: c4cdbce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions

Copy link
Copy Markdown
Contributor

📚 Branch Preview Links

🔍 Gen1 Visual Regression Test Results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Deployed to Azure Blob Storage: pr-6561

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

miwha-adobe and others added 3 commits July 30, 2026 09:40
Restore Tabs, AlertBanner, and Meter to the PR 2 (components) baseline
so this PR carries only the philosophy-neutral work (lifecycle
placement normalization + the validateEnum retrofit) and does not ship
the contested coerce-to-default removal.

The fallback-vs-warn-only decision is moving to its own ticket for team
discussion; the removal lives on miwha/refactor-dev-warning-fallback-poc
as a proof-of-concept to attach there.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The 1st-gen renderIcon change (return `nothing` instead of empty
templates) was committed to this 2nd-gen dev-warning branch by mistake.
This branch should not touch 1st-gen files. Restore both 1st-gen
AlertBanner files to the PR 2 baseline so the PR carries only 2nd-gen
dev-warning placement work.

The original change is preserved in commit 2603a2a if it is wanted
on its own branch later.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@miwha-adobe miwha-adobe changed the title miwha/feat dev warning refactors feat(validation): lifecycle placement of dev warnings Jul 30, 2026
@miwha-adobe
miwha-adobe marked this pull request as ready for review July 30, 2026 16:56
@miwha-adobe
miwha-adobe requested a review from a team as a code owner July 30, 2026 16:56
@miwha-adobe miwha-adobe added Status:Ready for review PR ready for review or re-review. gen2 These issues or PRs map to our 2nd generation work to modernizing infrastructure. labels Jul 30, 2026

@cdransf cdransf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! ✨

@rubencarvalho rubencarvalho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just one little change request 😄

}
}

if (changes.has('staticColor') && this.staticColor !== undefined) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubencarvalho
rubencarvalho dismissed their stale review August 6, 2026 13:35

not requesting changes, commenting only

@nikkimk nikkimk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment for discussion that i'll drop into the thread. If we decide to change course, we can always merge this and change direction in a follow up PR.

* class MyComponent extends SpectrumElement {
* private nameObserver = new AttributeObserverController(
* this,
* ['aria-label', 'aria-labelledby'],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of cross root ARIA issues, hosts shouldn't be using the aria-label and aria-labelledby attributes directly. Instead we typically have a label attribute that sets this. I'm wondering if, rather than creating a controller here, we align the API to have label and labelledby properties that set aria-label and aria-labelledby, even in components where the host has the role.

@@ -0,0 +1,212 @@
/**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for using a demo component to illustrate this instead of an SWC component. It provides a simple component for us to understand how to implement and it helps us isolate issues to the controller or component. <3

super.updated(changedProperties);
if (changedProperties.has('variant')) {
const constructor = this.constructor as typeof TooltipBase;
validateEnum(this, {

@Rajdeepc Rajdeepc Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bug: I see Tooltip and ProgressCircle fold the validateEnum check in willUpdate while others do in update. Again, functionally both is running pre-render but its an inconsistency where we are trying to create a pattern.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this controller? It feels like a big overhead for what could (maybe) be simpler... e.g., the platform already gives us attributeChangedCallback/observedAttributes, which is what Lit uses internally to sync attributes to properties, and would get us attribute-change detection without a MutationObserver instance per host (even though I only see it being used on Dropzone for now? - which may also hint at some premature optimization).

But even setting that aside (and following up our Slack thread), we already have a first-party pattern for this, using accessible-label / accessibleLabel. If we lean on that convention instead of watching aria-label directly, we also won't need the controller for this case.

Aaaand as a bit of a tangent, but what I think we haven't decided / established a pattern is for labelledby. We've been treating aria-labelledby as the attribute... Should we extend the accessible-label pattern with an accessible-labelledby counterpart too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gen2 These issues or PRs map to our 2nd generation work to modernizing infrastructure. Status:Ready for review PR ready for review or re-review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants